Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Control block forging through NodeKernel #140

Merged
merged 15 commits into from
Jul 3, 2023
Merged

Control block forging through NodeKernel #140

merged 15 commits into from
Jul 3, 2023

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Jun 7, 2023 •
edited
Loading

This PR supersedes IntersectMBO/ouroboros-network#3800 and regards issue IntersectMBO/ouroboros-network#3159.

I mostly just "rebased" the old ouroboros-network branch on top of this new repo. Please look at the discussions in the old PR for more details.

This PR is co-authored-by: Marcin Szamotulski coot@coot.me @coot

@bolt12 bolt12 requested a review from a team as a code owner June 7, 2023 14:00
@bolt12 bolt12 mentioned this pull request Jun 7, 2023
Closed
9 tasks
@bolt12 bolt12 self-assigned this Jun 7, 2023
@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch 2 times, most recently from d52de2e to 982a1d9 Compare June 12, 2023 10:14
This was referenced Jun 12, 2023
Closed
Closed
@coot coot requested a review from nfrisby June 14, 2023 14:23
@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch from a4e3479 to a888016 Compare June 22, 2023 08:49
jasagredo
jasagredo reviewed Jun 22, 2023
View reviewed changes
...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs Show resolved Hide resolved
...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs Outdated Show resolved Hide resolved
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs Outdated Show resolved Hide resolved
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs Show resolved Hide resolved
...ros-consensus-diffusion/changelog.d/20230612_124926_armandoifsantos_dynamic_block_forging.md Outdated Show resolved Hide resolved
@bolt12 bolt12 requested a review from jasagredo June 22, 2023 09:47
dnadales
dnadales reviewed Jun 22, 2023
View reviewed changes
ouroboros-consensus-cardano/src/byron-testlib/Ouroboros/Consensus/ByronDual/Node.hs
@@ -82,9 +82,11 @@ protocolInfoDualByron :: forall m. Monad m
=> ByronSpecGenesis
-> PBftParams
-> [CoreNodeId] -- ^ Are we a core node?
-> ProtocolInfo m DualByronBlock
-> ( ProtocolInfo DualByronBlock
, m [BlockForging m DualByronBlock]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we return two things I wonder if we should rename this function.

Also, we should add a comment explaining what the second component of the tuple is, and maybe how it relates to the input parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe but that would be a very disruptive change, wouldn't it?

@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch from a888016 to a01bf12 Compare June 22, 2023 11:23
@nfrisby
Copy link
Contributor

nfrisby commented Jun 22, 2023

A higher-level note from our mob call reviewing this PR:

Our concern with this PR is confined to ensuring (via brackets, masks, etc) that the node can cleanly shutdown. For that argument, we can assume that the ResourceRegistry is "managing the lifetimes correctly". For example: when a thread is calling its clean-up logic, all younger threads are either already dead or else (uninterruptibly) masked and will die as soon as they are unblocked (eg by this thread's clean-up logic).

Clean shutdown is not necessarily the only concern. However, we are deferring anything beyond clean shutdown to this pre-existing issue: #543

@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch from a01bf12 to 85a2441 Compare June 22, 2023 16:45
@bolt12 bolt12 requested a review from dnadales June 22, 2023 16:45
@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch 2 times, most recently from e0d484a to d639fbd Compare June 23, 2023 12:01
@bolt12 bolt12 mentioned this pull request Jun 23, 2023
Merged
@coot coot linked an issue Jun 27, 2023 that may be closed by this pull request
Enable block production dynamically IntersectMBO/ouroboros-network#3159
Closed
bolt12 and others added 14 commits June 30, 2023 17:19
@bolt12 @coot
Added setBlockForging to NodeKernel
1981ee6 
Block forging is removed from ProtocolInfo, and can controlled by using
`NodeKernel` field: `setProtocolForging :: [BlockForging m blk] -> m ()`.

Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-test
f465e7e 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-mock
be0ec86 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-mock-test
d1e067c 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-byron
4b90ea4 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-byron-test
75bbcda 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-shelley
7e2933e 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-shelley-test
07588ac 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-cardano library
7039cff 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-cardano:db-analyzer
a245233 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Updated ouroboros-consensus-cardano-test
b974754 
Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
Make forkBlockForging async safe
cd40291 
When make sure that when a block is added to the ChainDB, transactions
will be removed from the mempool.   The 'addBlockAsync' is a lightweight
non-blocking operation but the finalizer is blocking (`blockProcessed`
will block until the block was added to the ChainDB).  Hence we need to
use `uninterruptibleMask_` to make it safe in presence of asynchronous
exceptions.

Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12 @coot
block-forging: async exception safety
5b4d52a 
When the block forger thread adds a new block, the adding thread might
be killed by an async exception.  If that happens, the block forger will
get 'Nothing' when `blockProcessed` returns, and it can exit.

Co-authored-by: Marcin Szamotulski <coot@coot.me>
@bolt12
Added Changelog entries
e22b424
@bolt12 bolt12 force-pushed the bolt12/dynamic-block-forging branch from d639fbd to e22b424 Compare June 30, 2023 16:19
@bolt12 bolt12 enabled auto-merge June 30, 2023 16:20
@bolt12 bolt12 added this pull request to the merge queue Jul 3, 2023
Merged via the queue into main with commit 1cb0b79 Jul 3, 2023
@bolt12 bolt12 deleted the bolt12/dynamic-block-forging branch July 3, 2023 09:17
@dnadales dnadales mentioned this pull request Jul 3, 2023
Closed
16 tasks
@coot coot mentioned this pull request Jul 7, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadales dnadales dnadales approved these changes

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby is a code owner

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo is a code owner

Assignees

@bolt12 bolt12

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable block production dynamically
4 participants
@bolt12 @nfrisby @dnadales @jasagredo